Skip to content

fix: groups and permissions should not be changed when retrieving #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: groups and permissions should not be changed when retrieving #37

wants to merge 4 commits into from

Conversation

landerlini
Copy link

@landerlini landerlini commented Jul 22, 2025 •
edited by coderabbitai bot
Loading

Fixes #34

Added flags --no-o, --no-g and --no-p to rsync at data retrieval as done when uploading data.

Summary by CodeRabbit

  • Bug Fixes
    • Adjusted object retrieval to stop preserving owner, group, and permission metadata when copying files locally, preventing permission and ownership conflicts across systems.
    • No changes to public interfaces or visible behavior beyond more consistent file permissions when pulling stored files.

@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Jul 22, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The StorageObject.retrieve_object implementation was changed to add rsync flags that disable preserving owner, group, and permissions when copying non-on-demand objects; no public signatures or control flow were modified.

Changes

Cohort / File(s) Change Summary
Storage retrieval logic
snakemake_storage_plugin_fs/__init__.py		 Updated rsync invocation in StorageObject.retrieve_object to include --no-o, --no-g, and --no-p when copying non-on-demand objects.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely summarizes the primary change of preventing modifications to groups and permissions during file retrieval by adding the appropriate rsync flags, which directly reflects the main aim of the pull request.
Linked Issues Check ✅ Passed The changes implement the core requirement from issue #34 by adding the --no-o, --no-g, and --no-p flags to the retrieve_object rsync invocation, ensuring that ownership, group, and permissions are no longer preserved on retrieval as specified.
Out of Scope Changes Check ✅ Passed All modifications are confined to the retrieve_object method’s rsync invocation to disable ownership, group, and permission preservation, with no unrelated files or logic altered, so there are no out-of-scope changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 19a5cdf and aa3e0e5.

📒 Files selected for processing (1)
  • snakemake_storage_plugin_fs/__init__.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • snakemake_storage_plugin_fs/__init__.py
🔇 Additional comments (1)
snakemake_storage_plugin_fs/__init__.py (1)

214-219: LGTM! Fix correctly addresses permission issues during retrieval.

The addition of --no-o, --no-g, and --no-p flags prevents rsync from attempting to preserve ownership, group, and permissions during file retrieval. This resolves the issue where processes lacking sufficient privileges would fail when retrieving files owned by other users. The change also creates consistency with the store_object method (line 240), which already uses these same flags.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fgvieira fgvieira fgvieira approved these changes

@johanneskoester johanneskoester Awaiting requested review from johanneskoester

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Please (optionally?) ignore user and group of the original file in the shared file system when retrieving

2 participants

@landerlini @fgvieira